[Python] Set memory policy to "strict" #13593

guitargeek · 2023-09-04T13:04:33Z

The ROOT Python interfaces have many memory leaks, which is a major pain
point for people using it for long-running scripts in batch jobs.

One source of memory leaks was indentified to be the "heuristic memory
policy" of cppyy. This means that cppyy assumes that every non-const
pointer member function argument was interpreted as the object taking
ownership if the argument.

For examle, take the non-owning RooLinkedList container. It has a
RooLinkedList::Add(RooAbsArg *arg) method. ROOT wrongly assumes that
this means the RooLinkedList takes ownership of arg, and it drops the
ROOT overship. Nobody feels responsible for deleting the object
anymore, and there is a memory leak or arg.

That particular leak was reported in this forum post:
https://root-forum.cern.ch/t/memory-leak-in-fits/56249

Function parameters of type T * are very common in ROOT, and only
rarely do they imply ownership transfer. So changing the memory policy
to "strict" would surely fix also many other memory leaks that are not
reported so far. In fact, upstream cppyy doesn't even have this
heuristic memory policy anymore! So moving ROOT also to the strict
memory policy closes the gap between ROOT and cppyy.

The potential drawback of this change are crashes in usercode if memory
is not properly managed. But these problems should either be fixed by:

the user
dedicated pythonizations for these methods to manage shared
ownership via Python reference counters (i.e., setting the parameter
as an attribute of the object that the member function was called
on)

This follows up on PR #4294, in particular it reverts guitargeek@3a12063.

Sister PR of root-project/root#13593.

vepadulano

I'm coming back to this old PR, I still agree with my previous statement that this is the right direction, but I see more potential misunderstandings and issues creeping up. I'm thinking about approaches we could employ to mitigate them, e.g. by running also these changes in CMSSW CI, or by thinking about more Pythonizations to include (e.g. in TList), or by thinking about if we can add more warnings for users in situations that lead to dangling pointers.

Also, a question that becomes more important now, are all of these changes still required in light of the recent and future improvements to the cppyy we use?

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py

bindings/pyroot/pythonizations/test/tcollection_listmethods.py

vepadulano · 2026-01-07T14:45:09Z

bindings/pyroot/pythonizations/test/tseqcollection_listmethods.py

+        # A TList is by default non-owning. To make sure that the objects live
+        # long enough, we attach then as an attribute of the output list, such
+        # that the Python reference counter doesn't hit zero.
+        sc.owning_pylist = sc_pylist


This sounds counter-intuitive for a Python user, shall we perhaps discuss making it part of a TList pythonization?

README/ReleaseNotes/v636/index.md

vepadulano · 2026-01-07T14:48:06Z

README/ReleaseNotes/v636/index.md

+**Note:** You can change back to the old policy by calling
+`ROOT.SetMemoryPolicy(ROOT.kMemoryHeuristics)` after importing ROOT, but this
+should be only used for debugging purposes and this function might be removed
+in the future!


We should make this perhaps explicit at usage by also raising a warning from the function when calling it.

guitargeek · 2026-01-07T15:34:11Z

Hi @vepadulano, thanks for coming back to this! Let's try to make progress then and merge this soon 🙂

I'm coming back to this old PR, I still agree with my previous statement that this is the right direction, but I see more potential misunderstandings and issues creeping up. I'm thinking about approaches we could employ to mitigate them, e.g. by running also these changes in CMSSW CI, or by thinking about more Pythonizations to include (e.g. in TList), or by thinking about if we can add more warnings for users in situations that lead to dangling pointers.

Good points. I'll address them once we see where we stand with a fresh CI run.

Also, a question that becomes more important now, are all of these changes still required in light of the recent and future improvements to the cppyy we use?

The upcoming cppyy rebase on libinterop is more happening in the backend, and we have no frontend development lined up that change the behavior of these memory heuristics. But what we should consider is that it's better to not change both backend and frontend too much in one release, because it makes potential regressions harder to fix. If we assume that the backend upgrade will hit in 6.42, then this means the best time to update the memory policy is now for 6.40. Otherwise, it would be wiser to wait for 6.44, but this is quite far away.

guitargeek · 2026-01-09T16:45:29Z

Hi @vepadulano, I have suggested a TList Pythonization now that might address your concerns.

The proposal is that when adding to owning TCollections (checked with IsOwner()), the ownership is always relinquished on the Python side, just like the old memory heuristics implied.

So that means that the behavior change is only for non-owning TCollections, which now add borrowed references instead of letting the elements leak when added from Python.

Is that an acceptable compromise? (no need to rush an answer, I still have to fix test failures with Python 3.14 for Fedora Rawhide 🙂 )

The ROOT Python interfaces have many memory leaks, which is a major pain point for people using it for long-running scripts in batch jobs. One source of memory leaks was indentified to be the "heuristic memory policy" of cppyy. This means that cppyy assumes that every non-const pointer member function argument was interpreted as the object taking ownership if the argument. For examle, take the non-owning RooLinkedList container. It has a `RooLinkedList::Add(RooAbsArg *arg)` method. ROOT wrongly assumes that this means the RooLinkedList takes ownership of arg, and it drops the ROOT overship. Nobody feels responsible for deleting the object anymore, and there is a memory leak or `arg`. That particular leak was reported in this forum post: https://root-forum.cern.ch/t/memory-leak-in-fits/56249 Function parameters of type `T *` are very common in ROOT, and only rarely do they imply ownership transfer. So changing the memory policy to "strict" would surely fix also many other memory leaks that are not reported so far. In fact, upstream cppyy doesn't even have this heuristic memory policy anymore! So moving ROOT also to the strict memory policy closes the gap between ROOT and cppyy. The potential drawback of this change are crashes in usercode if memory is not properly managed. But these problems should either be fixed by: * the user * dedicated pythonizations for these methods to manage shared ownership via Python reference counters (i.e., setting the parameter as an attribute of the object that the member function was called on) This follows up on PR root-project#4294, in particular it reverts 3a12063.

vepadulano

Thanks! I like the direction, I left a few more comments/questions.

vepadulano · 2026-01-12T09:46:48Z

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_memory_utils.py

+
+def declare_cpp_owned_arg(position, name, positional_args, keyword_args, condition=lambda _: True):
+    """
+    Helper function to drop Python ownership of a specific funciton argument


Suggested change

Helper function to drop Python ownership of a specific funciton argument

Helper function to drop Python ownership of a specific function argument

vepadulano · 2026-01-12T09:48:34Z

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_memory_utils.py

+    """
+    import ROOT
+
+    arg = None


This is not required in Python, unless you expect sometimes the two if conditions to never trigger thus arg would not be defined. If this is a possibility, then I wonder if condition(arg) could be ill-defined, depending on whether the function condition behaves well with None

vepadulano · 2026-01-12T09:51:24Z

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tgraph.py

+    The TMultiGraph takes ownership of the added graphs, unless we're using the
+    Add(TMultiGraph*) overload, in which cases it adopts the graphs inside the
+    other TMultiGraph.


From this phrasing it seems that also for the Add(TMultiGraph*) the TMultiGraph takes ownership, isn't it so? I don't understand if you mean that in that case the TMultiGraph is not owning the graphs.

vepadulano · 2026-01-12T09:53:41Z

bindings/pyroot/pythonizations/test/tseqcollection_itemaccess.py


    def test_getitem_slice(self):
        sc = self.create_tseqcollection()
+        sc.SetOwner(False)


The fact you are now calling both SetOwner(True) and SetOwner(False) in different unittests confuses me as to which is the default behaviour (in C++ and Python) of the TCollection family of classes.

Especially important for documentation and to address potential future user questions

vepadulano · 2026-01-12T09:56:28Z